Renamed count headings in schema summary of html/txt report and fixed all cases to add to invalid count #2073

priyanshi-yb · 2024-12-12T12:56:46Z

Describe the changes in this pull request

1.Changes in the Analyze schema HTML/TXT reports and Assess-migration HTML report for
Total Count -> Total Objects
Valid Count -> Object Without Issues
Invalid Count -> Object With Issues
2. Fixed all the reporting issue cases to add to the invalid count based on object names
3. Fixed all the test cases expected files for this

Fixes

Describe if there are any user-facing changes

Changes in the Analyze schema HTML/TXT reports and Assess-migration HTML report for
Total Count -> Total Objects
Valid Count -> Object Without Issues
Invalid Count -> Object With Issues

How was this pull request tested?

Manually tested the HTML and TXT formats

Does your PR have changes that can cause upgrade issues?

Component	Breaking changes?
MetaDB	No
Name registry json	No
Data File Descriptor Json	No
Export Snapshot Status Json	No
Import Data State	No
Export Status Json	No
Data .sql files of tables	No
Export and import data queue	No
Schema Dump	No
AssessmentDB	No
Sizing DB	No
Migration Assessment Report Json	No
Callhome Json	No
YugabyteD Tables	No
TargetDB Metadata Tables	No

…eports of analyze schema 2. Fixed all the reporting issue case to add to the invalid count based on object names

makalaaneesh

LGTM with comments

makalaaneesh · 2024-12-17T08:04:03Z

migtests/tests/analyze-schema/expected_issues.json

@@ -371,16 +371,6 @@
            "GH": "https://github.com/YugaByte/yugabyte-db/issues/1131",
            "MinimumVersionsFixedIn": null
        },
-        {


how come this went away?

as discussed -
As I now added invalid count for this regex -

else if spc := alterViewRegex.FindStringSubmatch(sqlInfo.stmt); spc != nil { summaryMap["VIEW"].invalidCount[sqlInfo.objName] = true reportCase(fpath, "ALTER VIEW not supported yet.", "https://github.com/YugaByte/yugabyte-db/issues/1131", "", "VIEW", spc[1], sqlInfo.formattedStmt, UNSUPPORTED_FEATURES, "") }

and hence this is not going in this condition -

_, err := queryparser.Parse(sqlStmtInfo.stmt) if err != nil { //if the Stmt is not already report by any of the regexes if !summaryMap[objType].invalidCount[sqlStmtInfo.objName] { reason := fmt.Sprintf("%s - '%s'", UNSUPPORTED_PG_SYNTAX, err.Error()) reportCase(fpath, reason, "https://github.com/yugabyte/yb-voyager/issues/1625", "Fix the schema as per PG syntax", objType, sqlStmtInfo.objName, sqlStmtInfo.formattedStmt, UNSUPPORTED_FEATURES, "") }

makalaaneesh · 2024-12-17T08:04:46Z

migtests/tests/analyze-schema/summary.json

@@ -8,6 +8,7 @@
            {
                "ObjectType": "SCHEMA",
                "TotalCount": 1,
+                "InvalidCount": 1,


What is the logic for determining when a SCHEMA is invalid?

it is this Create schema with elements regex case -

CREATE SCHEMA hollywood CREATE TABLE films (title text, release date, awards text[]) CREATE VIEW winners AS SELECT title, release FROM films WHERE awards IS NOT NULL; createSchemaRegex.MatchString(sqlInfo.stmt) { summaryMap["SCHEMA"].invalidCount[sqlInfo.objName] = true reportCase(fpath, "CREATE SCHEMA with elements not supported yet.",

yb-voyager/cmd/analyzeSchema.go

makalaaneesh · 2024-12-17T08:30:33Z

yb-voyager/src/query/queryissue/issues_ddl.go

-	return newQueryIssue(deferrableConstraintIssue, objectType, objectName, sqlStatement, map[string]interface{}{})
+func NewDeferrableConstraintIssue(objectType string, objectName string, sqlStatement string, constraintName string) QueryIssue {
+	details := map[string]interface{}{
+		"ConstraintName": constraintName,


use a constant for this.

sanyamsinghal · 2024-12-18T07:10:17Z

yb-voyager/cmd/templates/schema_analysis_report.txt

+Object Type                    : {{ .ObjectType }}
+  - Total Count                : {{ .TotalCount }}
+  - Valid Count                : {{ sub .TotalCount .InvalidCount }}
+  - Objects With Issues Count  : {{ .InvalidCount }}


Lets name it Objects With Issues, Count is unnecessary.
Also lets rename InvalidCount variable to ObjectsWithIssues ?

But before renaming InvalidCount we need to see if the json payloads sent to yugabyted and callhome would also change. Are we using the InvalidCount field directly there?

I suggest renaming
TotalCount -> TotalObjects
ValidCount -> ObjectsWithoutIssues
InvalidCount -> ObjectWithIssues

Also lets rename InvalidCount variable to ObjectsWithIssues ?

No, we discussed this and didn't do it intentionally as it will effect json struct and will then effect yugabyted and callhome as we use it as is.
This is just UI change for HTML and TEXT reports only

we discussed this and didn't do it intentionally as it will effect json struct and will then effect yugabyted and callhome as we use it as is.

yeah but lets add a TODO, so that whenever we bump up the version of yugabyted payload we can consider changing this also.

sanyamsinghal · 2024-12-18T07:12:07Z

yb-voyager/src/query/queryissue/issues_ddl.go

@@ -254,10 +236,6 @@ var alterTableAddPKOnPartitionIssue = issue.Issue{

 func NewAlterTableAddPKOnPartiionIssue(objectType string, objectName string, SqlStatement string) QueryIssue {


nit: SqlStatement -> sqlStatement

sanyamsinghal · 2024-12-18T07:46:50Z

yb-voyager/cmd/analyzeSchema.go

+	var constraintIssues = []string{
+		queryissue.EXCLUSION_CONSTRAINTS,
+		queryissue.DEFERRABLE_CONSTRAINTS,
+		queryissue.PK_UK_ON_COMPLEX_DATATYPE,
+	}
+	/*
+		TODO:
+		// unsupportedIndexIssue
+		// ObjectType = INDEX
+		// ObjectName = idx_name ON table_name
+		// invalidCount.Type = INDEX
+		// invalidCount.Name = ObjectName (because this is fully qualified)
+		// DisplayName = ObjectName
+
+		// deferrableConstraintIssue
+		// ObjectType = TABLE
+		// ObjectName = table_name
+		// invalidCount.Type = TABLE
+		// invalidCount.Name = ObjectName
+		// DisplayName = table_name (constraint_name) (!= ObjectName)
+
+		// Solutions
+		// 1. Define a issue.ObjectDisplayName
+		// 2. Keep it in issue.Details and write logic in UI layer to construct display name.
+	*/
+	displayObjectName := issueInstance.ObjectName
+
+	constraintName, ok := issueInstance.Details[queryissue.CONSTRAINT_NAME]
+	if slices.Contains(constraintIssues, issueInstance.Type) && ok {
+		//In case of constraint issues we add constraint name to the object name as well
+		displayObjectName = fmt.Sprintf("%s, constraint: (%s)", issueInstance.ObjectName, constraintName)


building displaying name can be a different logic for different issue type.

How about having a function queryissue.GetDisplayObjectName() for the time being.

func GetDisplayObjectName(issueInstance QueryIssue) string { }

We might need this display name in the assess migration also? How are we dealing it in assess right now?

I'm also sort of running into the need for this with regex functions. I want to display the function names used...

But I think this needs a bit of discussion, there are so many fields out there 😅 it has become confusing. Shall we take this up separately @sanyamsinghal @priyanshi-yb ?

Sure. Agree, its becoming a bit confusing.

priyanshi-yb added 7 commits December 12, 2024 18:13

1. Rename the Invalid Count heading in schema summary of HTML / TXT r…

9f81b52

…eports of analyze schema 2. Fixed all the reporting issue case to add to the invalid count based on object names

fixed the analyze schema and assessment report testtests

b72042e

fixed complex schema expected files

fc769f4

Merge branch 'main' into priyanshi/invalid-cnt-rename-fix

0dc8f90

fix in reports after merge

84d16e1

minor fix in analyze

40d335c

Merge branch 'main' into priyanshi/invalid-cnt-rename-fix

333a7e1

priyanshi-yb marked this pull request as ready for review December 13, 2024 06:09

priyanshi-yb requested a review from makalaaneesh December 13, 2024 06:09

priyanshi-yb mentioned this pull request Dec 13, 2024

Reporting the Advisory locks, XML Functions and System columns on the DDLs in the unsupported features in assess/analyze #2061

Merged

minor template fix for text report

b04558a

priyanshi-yb force-pushed the priyanshi/invalid-cnt-rename-fix branch from 9d84e02 to b04558a Compare December 13, 2024 11:32

priyanshi-yb added 2 commits December 16, 2024 14:25

Merge branch 'main' into priyanshi/invalid-cnt-rename-fix

fa9d2da

fix unit test

e90f128

makalaaneesh approved these changes Dec 17, 2024

View reviewed changes

review comments

08cd5a1

sanyamsinghal reviewed Dec 18, 2024

View reviewed changes

review comments

55812bf

priyanshi-yb changed the title ~~Rename Invalid count to Objects with issues count in html/txt analyze report and fixed all cases to add to invalid count~~ Renamed count headings in schema summary of html/txt report and fixed all cases to add to invalid count Dec 18, 2024

priyanshi-yb merged commit 736fcee into main Dec 18, 2024
42 checks passed

priyanshi-yb deleted the priyanshi/invalid-cnt-rename-fix branch December 18, 2024 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renamed count headings in schema summary of html/txt report and fixed all cases to add to invalid count #2073

Renamed count headings in schema summary of html/txt report and fixed all cases to add to invalid count #2073

priyanshi-yb commented Dec 12, 2024 •

edited

Loading

makalaaneesh left a comment

makalaaneesh Dec 17, 2024

priyanshi-yb Dec 17, 2024

makalaaneesh Dec 17, 2024

priyanshi-yb Dec 17, 2024

makalaaneesh Dec 17, 2024

sanyamsinghal Dec 18, 2024

sanyamsinghal Dec 18, 2024

sanyamsinghal Dec 18, 2024

priyanshi-yb Dec 18, 2024 •

edited

Loading

sanyamsinghal Dec 18, 2024 •

edited

Loading

sanyamsinghal Dec 18, 2024

sanyamsinghal Dec 18, 2024

makalaaneesh Dec 18, 2024

sanyamsinghal Dec 18, 2024

priyanshi-yb Dec 18, 2024

		@@ -254,10 +236,6 @@ var alterTableAddPKOnPartitionIssue = issue.Issue{

		func NewAlterTableAddPKOnPartiionIssue(objectType string, objectName string, SqlStatement string) QueryIssue {

Renamed count headings in schema summary of html/txt report and fixed all cases to add to invalid count #2073

Renamed count headings in schema summary of html/txt report and fixed all cases to add to invalid count #2073

Conversation

priyanshi-yb commented Dec 12, 2024 • edited Loading

Describe the changes in this pull request

Describe if there are any user-facing changes

How was this pull request tested?

Does your PR have changes that can cause upgrade issues?

makalaaneesh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyanshi-yb Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

sanyamsinghal Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyanshi-yb commented Dec 12, 2024 •

edited

Loading

priyanshi-yb Dec 18, 2024 •

edited

Loading

sanyamsinghal Dec 18, 2024 •

edited

Loading